[improve][ml] Pin ledger callbacks to the managed ledger thread with withOrderingKey - #26599
Conversation
Every ledger open in managed-ledger still went through the legacy BookKeeper.asyncOpenLedger / asyncOpenLedgerNoRecovery overloads, while ledger creation already uses newCreateLedgerOp(). The builder API is where BookKeeper adds per-ledger options (4.18.1 ships OpenBuilder.withKeepUpdateMetadata and withOrderingKey), so this moves the remaining opens to newOpenLedgerOp() without changing behavior. The three opens that passed keepUpdateMetadata=true (ManagedLedgerImpl init-time open and concurrent-modification recheck, ManagedCursorImpl cursor-ledger recovery) use withRecovery(true).withKeepUpdateMetadata(true), which maps onto the same LedgerOpenOp.initiateWithKeepUpdateMetadata() path. The no-recovery opens in ShadowManagedLedgerImpl and the offline-stats cursor read in ManagedLedgerFactoryImpl use withRecovery(false). The existing OpenCallback bodies are kept and fed from the returned future, casting the handle to LedgerHandle as the real client returns a ReadOnlyLedgerHandle. PulsarMockBookKeeper's open builder returned a plain ReadHandle, which cannot be cast to the LedgerHandle the managed ledger and cursor keep. PulsarMockReadHandle is now a read-only LedgerHandle sharing the writer's entries, like ReadOnlyLedgerHandle: close is a no-op, reads still go through the read interceptor, it honors the mock's empty-ledger countdown, and the legacy asyncReadEntries is shared with the write handle. The mock's open builder completes on the mock executor, as the legacy mock path and the real client do. ManagedCursorTest and ManagedLedgerFactoryShutdownTest intercept newOpenLedgerOp() instead of the legacy overloads.
…withOrderingKey A managed ledger runs on bookKeeper.getMainWorkerPool().chooseThread(name), while the BookKeeper client pins each LedgerHandle to chooseThread(ledgerId) of the same pool. The two keys hash to different threads, so every add completion pays an extra hop from the ledger thread to the managed ledger thread, and each bookie response wakes a thread that is not the one doing the ledger's work. BookKeeper 4.18.1 added CreateBuilder/OpenBuilder.withOrderingKey (apache/bookkeeper#4881): the key is resolved with chooseThread(key) on the client's main worker pool, so passing the managed ledger name makes every callback of the handle run on the managed ledger thread. This passes the name at asyncCreateLedger (data and cursor ledgers) and at every open in ManagedLedgerImpl, ReadOnlyManagedLedgerImpl, ShadowManagedLedgerImpl and ManagedCursorImpl. The offline-stats opens in ManagedLedgerFactoryImpl have no managed ledger thread and are left unkeyed. ManagedLedgerBkTest gains a test against a real bookie that checks the read callbacks of the current ledger, of a closed ledger re-opened through getLedgerHandle, and of the cursor ledger all run on the managed ledger's executor thread.
The managed ledger now opens its last ledger and the cursor ledgers through newOpenLedgerOp() when a topic is reloaded, so the open-builder stub in this test also records those opens. Clear the recorded set right before compacting, and expect the penultimate ledger to have been opened for its stats after the reload, as the comment already said.
…d ledger thread With the ledger callbacks pinned to the managed ledger thread through withOrderingKey, OpAddEntry.addComplete, PendingReadsManager.attach and OpReadEntry.complete no longer need to re-queue their processing on the executor: ThreadBoundExecutor.executeOrRun runs it inline when already on that thread and falls back to execute() otherwise (mock clients still deliver callbacks from their own thread). The managed ledger executor is now typed as ThreadBoundExecutor, cast from the main worker pool thread the same way the BookKeeper client does for its ledger handles. Tests that stub getExecutor() hand out a ThreadBoundExecutor: a mock, a worker thread of the OrderedExecutor the mock client uses, or the new DirectThreadBoundExecutor for the test that needs same-thread completion.
… writes on the mock read view Review follow-ups: - The builder-based opens bridge their future to the legacy OpenCallback with whenComplete and dropped the returned stage, so anything the callback threw (for example a RejectedExecutionException when the managed ledger executor is already shut down) was swallowed instead of being logged the way BookKeeper's executor did. All six sites now go through ManagedLedgerImpl.completeOpenCallback, which logs it. - PulsarMockBookKeeper.newOpenLedgerOp() used thenComposeAsync, which short-circuits on an already failed programmed-failure future and completed the open inline on the caller's thread. It now always completes on the mock executor, as the legacy mock open path did. - PulsarMockReadHandle rejects writes with IllegalOpException, like ReadOnlyLedgerHandle, instead of falling through to LedgerHandle's write path and the mock's null client internals.
…dering-key # Conflicts: # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java
…dering-key # Conflicts: # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java # managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java
lhotari
left a comment
There was a problem hiding this comment.
Thanks for tracking down this hop and wiring withOrderingKey through — the mechanism itself checks out: withOrderingKey(name) really does land BookKeeper's callback dispatch on the same OrderedExecutor thread ManagedLedgerImpl already picks (verified against the pinned BookKeeper 4.18.1 sources, including that PerChannelBookieClient.executeOrdered uses the handle's own fixed executor and ignores ledgerId once one is supplied), and the unkeyed offline-stats opens in ManagedLedgerFactoryImpl are correctly left alone since they have no managed-ledger thread to pin to.
What needs another look is the switch from execute to executeOrRun in three completion paths — each of those removes a queue hop that was also acting as an implicit safety boundary, and in a few spots something now runs where it previously couldn't:
- OpReadEntry.complete() drops the trampoline that kept a cached-batch read loop from recursing on the same Java stack.
OpScan(cursor.scan, used by e.g.PersistentSubscription.analyzeBacklog) andPersistentReplicator.readEntriesCompleteboth drive their next batch synchronously from the read-complete callback, so a long run of cache-hit batches now nests one call per batch instead of yielding back to the executor loop between batches. Tellingly, PersistentReplicator.java already has a comment from an earlier fix (#26106) guarding a different branch against exactly this "cursor reads can complete inline (cache hit)" recursion hazard — the ordinary completion path at the bottom of the same method isn't similarly guarded. - PendingReadsManager.attach() is
synchronizedon thePendingRead, and when the storage future is already complete at attach time (e.g. a synchronous BK-client-shutdown failure), the inlineexecuteOrRunnow runs listener callbacks — including other cursors'/dispatchers'synchronized readEntriesComplete/Failed— while still holding that monitor, which is exactly what the method's own "isn't synchronized since that could lead to deadlocks" comments were guarding against one level up. - The same inline-under-a-lock shape shows up for shadow-topic writes: OpAddEntry.addComplete's executeOrRun call now runs nested inside ShadowManagedLedgerImpl's own
synchronizedadd path, and that nested call can reach asynchronized(PersistentTopic)method while an admin-triggered offload on the same topic takes the opposite lock order.
Details, evidence and suggested fixes are in the inline comments. None of this needs a rewrite of the approach — the ordering-key mechanism is sound — but the three executeOrRun call sites need either a narrower inline condition or an explicit trampoline/queue boundary reinstated where the callback can re-enter locked code or drive further reads.
…nder a lock Review follow-ups on the executeOrRun change: - OpReadEntry.complete() goes back to execute(): a fully cached read completes synchronously and callers such as OpScan and the replicator issue their next read from the callback, so running the completion inline nested one call stack per cached batch. The queue hop is the trampoline that bounds that recursion. - PendingRead.attach() now holds the PendingRead monitor only for the state transition and registers whenComplete outside it. An already completed handle runs that callback inline, and the listener callbacks must not run while holding the lock that addListener takes from dispatcher threads. - Shadow writes complete the add operation from inside the shadow ledger's own synchronized methods; initiateShadowWrite now queues the completion on the ledger thread, so OpAddEntry.run never runs under that monitor and cannot reach topic-level locks from there. Real BookKeeper completions keep the inline path. Tests: testScanFromLedgerThreadOverCachedEntries drives a 5000-entry, single-entry-batch scan from the managed ledger thread over a warm cache (it overflowed the stack before this fix), and testInlineAddCompletionsKeepOrder submits adds from the managed ledger thread against a real bookie and checks they complete in order through the inline path.
OpReadEntry.complete() goes back to running inline on the ledger thread, which is the point of the ordering key: a read completion should not pay a queue hop. The recursion that the queue used to bound (a fully cached read completes synchronously and OpScan or the replicator issue the next read from the callback) is bounded by a nesting depth instead: past MAX_NESTED_INLINE_COMPLETIONS inline completions the next one is queued, which unwinds the stack, and the chain resumes inline from the executor loop. ManagedLedgerImpl carries the depth counter; it is only touched from the ledger thread. The scan regression test now asserts that the stack depth seen by the last entries stays within that bound instead of staying flat.
The nesting cap for inline read completions is a property of the thread's stack, not of one managed ledger: several independent reads interleave on a ledger thread without nesting, and a thread serves several managed ledgers. Keep the depth in a FastThreadLocal in OpReadEntry instead of a field on ManagedLedgerImpl, so only actual nesting on the current thread counts toward the cap.
…dering-key # Conflicts: # managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/cache/RangeEntryCacheImplTest.java
lhotari
left a comment
There was a problem hiding this comment.
LGTM. The lock boundaries, bounded read-completion nesting, and added regression coverage address my concerns.
Stacked on #26598 (the diff includes that commit until it merges; review the second commit here).
Motivation
A managed ledger runs on
bookKeeper.getMainWorkerPool().chooseThread(name), while the BookKeeper client pins eachLedgerHandletochooseThread(ledgerId)of the same pool. The two keys hash to different threads, so every add completion pays an extra hop from the ledger thread to the managed ledger thread (OpAddEntry.addCompletere-submits toml.getExecutor()), and each of the E bookie responses per entry wakes a thread that is not the one doing that ledger's work.BookKeeper 4.18.1 added
CreateBuilder.withOrderingKeyandOpenBuilder.withOrderingKey(apache/bookkeeper#4881): the key is resolved withOrderedExecutor.chooseThread(key)on the client's main worker pool, so passing the managed ledger name makes every callback of the resulting handle (add, read and close completions, recovery, and the completion of the create/open itself) run on the managed ledger thread. Nothing changes when no key is given.Modifications
ManagedLedgerImpl.asyncCreateLedger:.withOrderingKey(name), which covers data ledgers and cursor ledgers (cursors create theirs through the same method).newOpenLedgerOp()since [improve][ml] Open ledgers through the BookKeeper builder API #26598; this adds.withOrderingKey(name)to the init-time open, the concurrent-modification recheck and the two read-handle cache opens inManagedLedgerImpl, the open inReadOnlyManagedLedgerImpl, both source-ledger opens inShadowManagedLedgerImpl, and.withOrderingKey(ledger.getName())to the cursor-ledger recovery open inManagedCursorImpl.ManagedLedgerFactoryImplhave no managed ledger thread and stay unkeyed.OpAddEntry.addComplete,PendingReadsManager.attachandOpReadEntry.completeuseThreadBoundExecutor.executeOrRuninstead ofexecute, so the write and read completions run inline instead of going through the executor queue (they still queue when invoked from another thread, as the mock client does). The managed ledger executor is typed asThreadBoundExecutor, cast from the main worker pool thread the same way the BookKeeper client does for its ledger handles.getExecutor()now hand out aThreadBoundExecutor: a mock, a worker thread of theOrderedExecutorthe mock client uses, or the newDirectThreadBoundExecutortest helper for same-thread completion.Verifying this change
This change added tests and can be verified as follows:
ManagedLedgerBkTest.testLedgerCallbacksRunOnManagedLedgerThreadruns against a real bookie and asserts that the read callbacks of the current data ledger, of a closed ledger re-opened throughgetLedgerHandle, and of the cursor ledger all run on the managed ledger's executor thread. Existing coverage:ManagedLedgerTest,ManagedCursorTest,ManagedLedgerBkTest,ManagedLedgerFactoryChangeLedgerPathTest,ShadowManagedLedgerImplTest,ReadOnlyManagedLedgerImplTest,ManagedLedgerFactoryShutdownTest.